Skip to content

Adds code to delete all the access if no access returned from Ldap and enable_synchronize_access_from_ldap=1, #PG-4868 #415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
AltamashShaikh merged 9 commits into from
Jan 13, 2026
Merged

Adds code to delete all the access if no access returned from Ldap and enable_synchronize_access_from_ldap=1, #PG-4868 #415

AltamashShaikh merged 9 commits into from
Jan 13, 2026

Conversation

@AltamashShaikh
Copy link
Contributor

@AltamashShaikh AltamashShaikh commented Jan 7, 2026

Description

Adds code to delete all the access if no access returned from Ldap and enable_synchronize_access_from_ldap=1

Issue No

#PG-4868

Steps to Replicate the Issue

  1. Enable sync access from ldap
  2. Map the access in Ldap for a user
  3. Login via that user
  4. Now remove all the access for that user in LDAP and try logging in, it will work
  5. Checkout this PR
  6. Logout and Login again with that user.

Checklist

  • [✔] Tested locally or on demo2/demo3?
  • [✔] New test case added/updated?
  • [NA] Are all newly added texts included via translation?
  • [NA] Are text sanitized properly? (Eg use of v-text v/s v-html for vue)
  • [✔] Version bumped?
  • [NA] I have understood, reviewed, and tested all AI outputs before use
  • [NA] All AI instructions respect security, IP, and privacy rules

james-hill-matomo
james-hill-matomo reviewed Jan 7, 2026
View reviewed changes
LdapInterop/UserSynchronizer.php Outdated
Comment on lines 204 to 208
} else {
$this->logger->log('warning', "UserSynchronizer::{func}: User '{user}' has no access in LDAP, but access synchronization is enabled.", array(
'func' => __FUNCTION__,
'user' => $piwikLogin
));
Copy link
Contributor

@james-hill-matomo james-hill-matomo Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AltamashShaikh Not sure if this is ready for review yet, but this error message is now wrong. Is there any case where accessSynchronization is disabled and this function is called? I don't think there should be - having the if is nice, but maybe it should be further up the stack?

Copy link
Contributor Author

@AltamashShaikh AltamashShaikh Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@james-hill-matomo Updated logic.

@james-hill-matomo
Copy link
Contributor

james-hill-matomo commented Jan 8, 2026

#416

@AltamashShaikh
Copy link
Contributor Author

AltamashShaikh commented Jan 9, 2026

@james-hill-matomo Thanks for the suggestion, I have applied them here and updated the test case to be check the behaviour more accurately.

@AltamashShaikh AltamashShaikh mentioned this pull request Jan 9, 2026
Closed
james-hill-matomo
james-hill-matomo previously approved these changes Jan 11, 2026
View reviewed changes
Copy link
Contributor

@james-hill-matomo james-hill-matomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you run me through testing it before we merge?

@AltamashShaikh
Copy link
Contributor Author

AltamashShaikh commented Jan 12, 2026

LGTM. Can you run me through testing it before we merge?

@james-hill-matomo to test it locally, you should have LDAP setup and 1 ldap user created.

  1. Add below code in plugins/LoginLdap/LdapInterop/UserAccessMapper.php Line 114 in getPiwikUserAccessForLdapUser to mimic the access issue locally.
if ($ldapUser['uid'] === 'johndoe') {
//            return []; // To test empty scenario uncomment ths line
            return array(
                'view' => array(1,2,3),
                'admin' => array(4,5,6)
            );
        }
  1. Sync the user from ldap screen
image 3. Login using that user and you should be able to have admin access for site 4,5,6 and view access for 1,2,3. 4. Now uncomment the empty array scenario and sync again 5. Try logging again with the ldap user and you would see this screen image

james-hill-matomo
james-hill-matomo approved these changes Jan 12, 2026
View reviewed changes
Copy link
Contributor

@james-hill-matomo james-hill-matomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New !empty($userAccess) check not necessary, but it doesn't hurt either.

If this was based on our discussion yesterday, I was meaning that the addition of "|| !Config::isAccessSynchronizationEnabled()" to line 193 in UserSynchronizer was confusing the code a little... but it also doesn't do any actual harm to the running programme either.

Anyway, it continues to be fine and this is all nitpicks, so continuing to approve :)

@AltamashShaikh AltamashShaikh merged commit 426f47f into 5.x-dev Jan 13, 2026
8 checks passed
@AltamashShaikh AltamashShaikh deleted the PG-4868-remove-access branch January 13, 2026 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@james-hill-matomo james-hill-matomo james-hill-matomo approved these changes

Assignees

No one assigned

Labels

Needs Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AltamashShaikh @james-hill-matomo